feat: consensus messages #141
Conversation
There was a problem hiding this comment.
LGTM! all comments are the same as for the rollapp-evm PR: dymensionxyz/rollapp-evm#351
app/app.go
Outdated
|
|
||
| theType, err := proto.Marshal(resp) | ||
| if err != nil { | ||
| return nil |
There was a problem hiding this comment.
why return nil? don't you need to append to responses and continue?
There was a problem hiding this comment.
need to verify if this type of error is expected
| _, isError := response.Response.(*abci.ConsensusMessageResponse_Error) | ||
| require.True(t, isError, "Expected an error response but got a success") |
There was a problem hiding this comment.
i'd also suggest verifying the returned error string
There was a problem hiding this comment.
@danwt does not like to check error strings
There was a problem hiding this comment.
yea but it's still better than nothing
There was a problem hiding this comment.
think in theory you can check grpc status code
i haven't looked at it tho
app/app.go
Outdated
| return resp | ||
| } | ||
|
|
||
| func (app *App) processConsensusMessage(ctx sdk.Context, consensusMsgs []*prototypes.Any) []*abci.ConsensusMessageResponse { |
There was a problem hiding this comment.
same comments as on the evm repo - and also as I wrote there - I suggest moving this method to common util on the RDK vs re-writing it (if possible) .
| @@ -827,6 +832,11 @@ func NewRollapp( | |||
| // upgrade. | |||
| app.setPostHandler() | |||
|
|
|||
| // Admission handler for consensus messages | |||
| app.setAdmissionHandler(consensus.AllowedMessagesHandler([]string{ | |||
| proto.MessageName(&banktypes.MsgSend{}), | |||
There was a problem hiding this comment.
why is this message allowed?
Description
ADR link https://www.notion.so/dymension/ADR-x-Sequencer-Messages-34f95e2b579e458e8bb4252da19324bd
Closes https://github.com/dymensionxyz/epics/issues/1
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
PR review checkboxes:
I have...
Unreleasedsection inCHANGELOG.mdgodoccommentsSDK Checklist
maptime.Now()sendCoinand notSendCoinsFull security checklist here
----;
For Reviewer:
---;
After reviewer approval: